[Fix_1578] malformed JSON from HTTP 200 is not normalized into WorkflowError error with Quarkus Rest client - #1579
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
The new WebApplicationException handling can misclassify malformed-JSON parsing failures as COMMUNICATION (potentially even with status 200) instead of the existing DATA(422) normalization, and it risks leaking the exception response if not closed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR aims to normalize client-side JSON parsing failures (notably with Quarkus REST client) into consistent WorkflowError instances, matching the behavior seen with the Jersey provider for malformed JSON in HTTP 200 responses.
Changes:
- Adds handling for
WebApplicationExceptionduring HTTP task execution to wrap it into aWorkflowException. - Uses the response status from
WebApplicationExceptionto build a communication-orientedWorkflowError.
File summaries
| File | Description |
|---|---|
| impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java | Adds WebApplicationException handling when invoking requests / reading entities to improve error normalization. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The new WebApplicationException handling can misclassify malformed JSON from successful (2xx) responses as a COMMUNICATION error with status 200 (and may leak the exception Response), which conflicts with the intended DATA/422 normalization.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java:71
- The new WebApplicationException handler maps successful (e.g., HTTP 200) responses that fail during entity parsing into a COMMUNICATION error with status 200. For malformed JSON from a 2xx response, this diverges from the existing ProcessingException path (Errors.DATA / 422) and from the issue’s expected normalization into a DATA workflow error. It also leaves the exception Response unclosed when invokeRequest/readEntity throws before the try-with-resources acquires a Response.
} catch (WebApplicationException ex) {
throw new WorkflowException(
WorkflowError.communication(ex.getResponse().getStatus(), task, ex).build(), ex);
}
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…owError error with Quarkus Rest client Signed-off-by: Gonzalo Muñoz <gmunozfe@redhat.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The updated exception mapping currently contains a compile-breaking WorkflowError.communication(Errors.DATA.toString(), ...) call and the WebApplicationException handling can produce a WorkflowError with HTTP 200 (and doesn’t close the embedded response), which conflicts with the #1578 normalization goal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java:66
- This maps
WebApplicationExceptionto a workflow error using the underlying response status verbatim; if the exception wraps a successful HTTP response (e.g., 200 with malformed JSON), this can generate a WorkflowError with status 200, which doesn’t satisfy the #1578 normalization expectation (422 for unparseable JSON). Also, theResponseinsideWebApplicationExceptionis not closed here, which can leak the entity stream/connection.
} catch (WebApplicationException ex) {
throw new WorkflowException(
WorkflowError.communication(ex.getResponse().getStatus(), task, ex).build(), ex);
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The new normalization drops the error detail payload compared to prior behavior and lacks a regression test for the malformed-JSON-on-200 scenario described in #1578.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (3)
impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java:66
- Same as above:
WorkflowError.communication(..., ex)sets the exception message astitlebut leavesdetailnull. Populatedetailas well so the error payload consistently contains details for response-processing failures.
} catch (WebApplicationException ex) {
throw new WorkflowException(
WorkflowError.communication(ex.getResponse().getStatus(), task, ex).build(), ex);
impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java:63
- This normalization no longer populates the WorkflowError
detailfield (previouslydetails(ex.getMessage()));WorkflowError.communication(..., ex)only sets the message as thetitle, leavingdetailnull. If callers / retry catch filters rely ondetail/details(and the issue’s expected JSON includesdetails), this is a behavior regression. Consider also setting.details(ex.getMessage())here.
This issue also appears on line 64 of the same file.
} catch (ProcessingException | IllegalStateException ex) {
throw new WorkflowException(
WorkflowError.communication(Errors.DATA.status(), task, ex).build(), ex);
impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java:66
- The new exception-to-WorkflowError normalization path isn’t covered by tests. Since there are already HTTP executor tests in
HTTPWorkflowDefinitionTest, add a test case for200 OK+Content-Type: application/json+ malformed JSON body asserting that aWorkflowExceptionis raised and the resultingWorkflowErroris normalized as intended (type/status and details/title).
} catch (ProcessingException | IllegalStateException ex) {
throw new WorkflowException(
WorkflowError.communication(Errors.DATA.status(), task, ex).build(), ex);
} catch (WebApplicationException ex) {
throw new WorkflowException(
WorkflowError.communication(ex.getResponse().getStatus(), task, ex).build(), ex);
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Co-authored-by: Francisco Javier Tirado Sarti <ftirados@ibm.com> Signed-off-by: Francisco Javier Tirado Sarti <ftirados@ibm.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The new WebApplicationException mapping can yield a WorkflowError with status=200 on parse failures (breaking downstream error filtering), and the behavior should be covered by a regression test for malformed JSON on HTTP 200.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (2)
impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java:66
- The
WebApplicationExceptionpath propagatesex.getResponse().getStatus()directly into theWorkflowError. If the client throwsWebApplicationExceptionwhile processing a 2xx response (e.g., malformed JSON on HTTP 200), this will produce an error withstatus=200, which is misleading and can break Catch/Try filtering that matches onerror.status()(seeTryExecutor.java:232). Also, switching from.details(ex.getMessage())tocommunication(..., ex)moves the exception message intotitleand leavesdetailnull, which changes what users can match on viaerror.details/detail. Consider normalizing successful-status exceptions to a non-2xx status (e.g.,Errors.DATA.status()) and populating.details(...)for parity with the previous behavior.
throw new WorkflowException(
WorkflowError.communication(Errors.DATA.status(), task, ex).build(), ex);
} catch (WebApplicationException ex) {
throw new WorkflowException(
WorkflowError.communication(ex.getResponse().getStatus(), task, ex).build(), ex);
impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java:66
- This change is addressing a client-specific failure mode (malformed JSON with
Content-Type: application/jsonon HTTP 200) but there doesn't appear to be an existing test covering the invalid-JSON/parse-failure path in the HTTP executor. Adding an integration test (similar to the otherMockWebServercases inHTTPWorkflowDefinitionTest) that returns a 200 with malformed JSON and asserts the resultingWorkflowExceptionhas a normalized non-2xxWorkflowError.status()would help prevent regressions across REST client implementations.
throw new WorkflowException(
WorkflowError.communication(Errors.DATA.status(), task, ex).build(), ex);
} catch (WebApplicationException ex) {
throw new WorkflowException(
WorkflowError.communication(ex.getResponse().getStatus(), task, ex).build(), ex);
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Fixes #1578